Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "fix: preserve mtime when zipping with the node zipper (#539)" #558

Merged

Conversation

bnegrao
Copy link
Contributor

@bnegrao bnegrao commented Oct 12, 2024

This PR reverts commit 162d9bc, which introduced non-deterministic behavior in the bundling process, leading to unnecessary re-deployments even when no code changes occurred. This change addresses issue #557.

@floydspace
Copy link
Owner

hi @bnegrao , thanks, I think it's fair to revert it, @iainlane we might need to have a better solution for this, like having configurtation option for zipper, for now I just let it revert

@iainlane
Copy link
Contributor

Any idea what the problem is? Is it to do with the target system or something?

@iainlane
Copy link
Contributor

A testcase to reproduce would be nice; the one which is being reverted here is asserting that the results are deterministic.

@floydspace floydspace merged commit 607a3ae into floydspace:master Oct 12, 2024
4 checks passed
Copy link

🎉 This PR is included in version 1.54.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@iainlane
Copy link
Contributor

Actually, I suspect what is happening is that generated files are updated on a re-package. Isn't that what is expected? The zip should be different if the input files have been updated (touched).

So - I think reverting this was the wrong move. The old behaviour wasn't really correct, and the new one was. If you're repackaging and that involves updating the generated code, you should indeed expect a different checksum on your zip.

In other words:

The zip files should be bundled in a "deterministic" way, that means, bundling the same code should always produce the exact same bundle.

Disagree that we are seeing "nondeterminism".

@floydspace
Copy link
Owner

according to the logic, the mtime is taken from the containing folder, meaning anything that is changed in the folder causes hash recalculation, I suppose, I could be wrong though.

@iainlane
Copy link
Contributor

I suspect if we took a look at the ls -lR of the zips we'd see the (generated) files have different mtimes.

Unless I understand this wrong (could be), we're taking the mtime of the source file itself - the file which is being added to the zip a few lines later. (On the next line we skip directories)

@floydspace
Copy link
Owner

floydspace commented Oct 12, 2024

Isn't that what is expected?

this is what I expect: no matter what changes are in the source code, if bundling produces the same bundle from content point of view, it should not trigger cloudformation changeset

@iainlane
Copy link
Contributor

Well if that's the position then you disagree with my initial change as far as I can see. When are people doing this kind of no-change repackage?

If you ask me I'd state it as this: if you run a no-op rebuild (why?) then you should expect to get a redeploy, because your files did change - even if just in modification time.

Which is a conflicting position to yours, and unless you find that convincing, as the maintainer your view should be the one to prevail.

It's no problem. I'll go and look for another way to deploy in which I can have working bundled static assets. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants